-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: port sports-league-scheduling quickstart example from Java to Python #685
base: development
Are you sure you want to change the base?
chore: port sports-league-scheduling quickstart example from Java to Python #685
Conversation
Thanks for the contribution, @PatrickDiallo23! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @PatrickDiallo23! I am impressed with the quality of the port. Some suggestions for improvement + consistency with the other quickstarts.
<head> | ||
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type"> | ||
<meta content="width=device-width, initial-scale=1" name="viewport"> | ||
<title>Sports League Scheduling - Timefold Solver on Python</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<title>Sports League Scheduling - Timefold Solver on Python</title> | |
<title>Sports League Scheduling - Timefold Solver for Python</title> |
.penalize(HardSoftScore.ONE_SOFT, home_away_matches_distance_lambda) | ||
.as_constraint("Away to home hop")) | ||
|
||
home_away_match_distance_lambda = lambda match: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if isinstance(match.home_team, Team) and isinstance(match.away_team, Team) else 0
should be unnecessary; all matches home and away teams should be set.
It might be best to add a method to Team get_distance(self, team: Team)
to perform the lookup logic.
def classic_matches(constraint_factory: ConstraintFactory) -> Constraint: | ||
return (constraint_factory | ||
.for_each(Match) | ||
.filter(is_invalid_classic_match) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would inline is_invalid_classic_match
@@ -0,0 +1,126 @@ | |||
import json | |||
import random |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent accidentally using a shared random, only import the Random
class from the package.
import random | |
from random import Random |
|
||
from .domain import * | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should create a seeded random so the generated demo data is consistent.
random = Random(0) |
distance_to_team: Annotated[Dict[str, int], | ||
DistanceToTeamValidator, | ||
Field(default_factory=dict)] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a get_distance(self, team: Team)
method here.
ProblemFactCollectionProperty, | ||
ValueRangeProvider] | ||
matches: Annotated[list[Match], | ||
PlanningEntityCollectionProperty] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly unaligned.
PlanningEntityCollectionProperty] | |
PlanningEntityCollectionProperty] |
class Round(JsonDomainBase): | ||
index: Annotated[int, PlanningId] | ||
# Rounds scheduled on weekends and holidays. It's common for classic matches to be scheduled on weekends or holidays. | ||
weekend_or_holiday: bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would annotate this attribute with Field(default=False)
so you wouldn't need to specify it every time in the test.
weekend_or_holiday: bool | |
weekend_or_holiday: Annotated[bool, Field(default=False)] |
classic_match=False) | ||
match2 = Match(id="2", home_team=home_team, away_team=rival_team, round=Round(index=0, weekend_or_holiday=False), | ||
classic_match=False) | ||
match3 = Match(id="3", home_team=home_team, away_team=rival_team, round=None, classic_match=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
round=None
and classic_match=False
are the defaults for Match
and do not need to be specified
(applies to the whole file wherever classic_match=False
or round=None
).
match3 = Match(id="3", home_team=home_team, away_team=rival_team, round=None, classic_match=False) | |
match3 = Match(id="3", home_team=home_team, away_team=rival_team) |
|
||
|
||
def test_classic_matches(): | ||
home_team = Team(id="1", name="TeamA", distance_to_team={}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to specify distance_to_team
if you are not populating it (it will create an empty dict using the default factory)
(applies to the whole file wherever distance_to_team={}
is used).
home_team = Team(id="1", name="TeamA", distance_to_team={}) | |
home_team = Team(id="1", name="TeamA") |
…-league-scheduling example
Hello @Christopher-Chianelli! I did another commit where I applied your code suggestions. Please review. Thank you! |
Description of the change
Port the sports-league-scheduling quickstart example from Java to Python while keeping the same functionality.
Checklist
Development
Code Review